Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add partitionByChunkSize method in CollectionUtils #269

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ramananravi
Copy link

No description provided.

@lespea
Copy link

lespea commented Nov 29, 2021

An empty collection shouldn't throw an exception it should just return an empty list...

@ramananravi
Copy link
Author

Thanks for the suggestion. Fixed it as suggested now.

@garydgregory
Copy link
Member

There are a lot of shorter ways to do this using Java 8 features: https://stackoverflow.com/questions/30995763/java-8-partition-list

@ramananravi
Copy link
Author

ramananravi commented Dec 1, 2021

There are a lot of shorter ways to do this using Java 8 features: https://stackoverflow.com/questions/30995763/java-8-partition-list

@garydgregory Thanks a lot for the suggestion. Fixed it now using Java 8 features as suggested. Please let me know in case of any other comments or better suggestions. Thanks again.

@garydgregory
Copy link
Member

@ramananravi
Note that Java has a method called subList(), so reusing that name might make sense.

@ramananravi
Copy link
Author

@ramananravi Note that Java has a method called subList(), so reusing that name might make sense.

@garydgregory Are you referring to the method name partitionByChunkSize? The reason for keeping this name is to keep this in sync with ListUtils.partition. Just to add, since partitionByNumOfChunks could be an extended version of this, kept this as partitionByChunkSize instead of just partition.
Please let me know if this sounds relevant. Also, wouldn't subList be specific to List (as we are having this util in CollectionUtils)?

@ramananravi
Copy link
Author

@kinow @garydgregory Got some quick questions since this is my first contribution to this repo.

  1. Would this PR be considered for merge as part of upcoming release?
  2. Is there any pending action item from my side on this PR?
    Thanks in advance.

@garydgregory
Copy link
Member

I'll take a look on Friday.

@ramananravi
Copy link
Author

I'll take a look on Friday.

Any updates on this? Also, in general, what's the PR merge and release cycle? Thanks in advance.

@garydgregory
Copy link
Member

Hi @ramananravi
Thanks for the ping.
We are all volunteers, the process is slow and from the outside may look random, and I am also busy being on the Log4j team. So patience is key ;-) The releases are fairly infrequent as this is a mature component. We are due for a release indeed but it is not high priority. I'll make it happen, eventually. HTH set expectations.

@ramananravi
Copy link
Author

@garydgregory thanks a lot for the explanation and sorry for the follow-up. Just wanted to know how the merge and release cycle works, just to plan things from my side accordingly. This helps. Thanks again for your time and support on this.

@garydgregory
Copy link
Member

garydgregory commented Dec 18, 2021 via email

@ramananravi
Copy link
Author

@garydgregory - extremely sorry for the follow-up. Please let me know if there are any updates on this? Thanks in advance.

@Claudenw
Copy link
Contributor

@ramananravi If you will fix the conflicting changes, I'll take another look and see if we can get this merged into the main branch.

@ramananravi
Copy link
Author

@ramananravi If you will fix the conflicting changes, I'll take another look and see if we can get this merged into the main branch.

@Claudenw - fixed as suggested. Please help check things and let me know in case of any further questions or comments. Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants